Introduce KEP 5975: Declarative API Definitions#5974
Introduce KEP 5975: Declarative API Definitions#5974jpbetz wants to merge 5 commits intokubernetes:masterfrom
Conversation
5e16441 to
194333b
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jpbetz The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
| without knowing their concrete types. Rather than relying on reflection, we would like to | ||
| introduce two interfaces: |
There was a problem hiding this comment.
I'm not fully convinced this is how I'd like to generically implement wiping. One alternative is reflection.
997fd43 to
e2cdb54
Compare
6649b90 to
5151b1a
Compare
99bd9c1 to
f4fe0c7
Compare
f4fe0c7 to
1245718
Compare
|
Note to self: Tests that verify DeepCopy, Conversion, and all other generators are run for each type would eliminate another aspect of manual API Review. |
0160c85 to
37d9d16
Compare
| ### Code generation | ||
|
|
||
| Rather than independently enable individual code generators with tags, code generation will | ||
| be on-by-default for all API definitions in code where declaration files exist. | ||
|
|
||
| `GroupVersion` will be placed in external API definition directories, For example: | ||
|
|
||
| `staging/src/k8s.io/api/admission/v1/groupversion.yaml`: | ||
|
|
||
| ```yaml | ||
| apiVersion: apidefinitions.k8s.io/v1alpha1 | ||
| kind: GroupVersion | ||
|
|
||
| group: admission.k8s.io | ||
| version: v1 | ||
| modelName: io.k8s.admission | ||
| ``` | ||
|
|
||
| This file defines all common properties of a group and it's presence indicates that | ||
| all external code generation, such as typed clients and openapi, should be run for this | ||
| group of resources. | ||
|
|
||
| `GroupVersionRegistration` will be placed in internal API registration directories, For example: | ||
|
|
||
| `pkg/apis/admission/v1/registration.yaml`: | ||
|
|
||
| ```yaml | ||
| apiVersion: apidefinitions.k8s.io/v1alpha1 | ||
| kind: GroupVersionRegistration | ||
|
|
||
| group: admission.k8s.io | ||
| version: v1 | ||
| externalTypes: k8s.io/api/admission/v1 | ||
| peers: | ||
| - k8s.io/kubernetes/pkg/apis/admission | ||
| ``` |
There was a problem hiding this comment.
@liggitt @thockin To simplify code generator management, I'm looking a way of enabling all code-generators by-default for in-tree APIs.
Initially I just tried adding a enforcement check that complained if an API author doesn't add all the +k8s:deepcopy-gen=true type tags, but @thockin mentioned look into driving the generators of small declarations. So I'm trying it out. This section shows what it would roughly look like.
Questions:
- Do we have an appetite for this sort of change? It will consume review bandwidth. The alternative would be to just merge something like Enable validation-gen and add check that all code generators always enabled kubernetes#138577.
- Are we okay with something where each directory that does API definition stuff has a file? Or do we want to take this in another direction (single file for an entire API resource?!)
There was a problem hiding this comment.
I'm ok with a file that describes which generator to run and how. Sounds similar to https://github.com/openshift/api/blob/master/example/.codegen.yaml in OpenShift. Some issues we've had:
- opt-in or opt-out or both. As the generators grow, it is sort of like admission. We have new items that contain default-on or default-off preferences and each API can have an "explicit off" or "explicit on" preference.
- Configuring at group level, across all versions made for fewer files, but more painful changes when we needed to start bringing new generators online.
- This became a natural place for validation and linting exceptions too. Not sure about your plan for growth there.
@JoelSpeed migrated us to the file and has been more hands on dealing with it recently.
There was a problem hiding this comment.
The idea we'd like to start with is:
- Here's a file that says everything code generators need to know about the API declaring files in in this directory
- When present, the generators run in a way that conforms to our API guidelines
I've got a PoC that shows this working against all in-tree APIs correctly and removes the need for a bunch of generator enablement tags in each doc.go file.
I do expect this to grow. I don't want it to accumulate validation/lint exceptions here.
For ecosystem adoption, I think it's fine to use outside of k/k. I would like it to focus on opinions we have for in-tree APIs. So if 3rd parties use it, they need to buy-in to our opinions.
bebb4c8 to
e955197
Compare
|
@alvaroaleman does kubebuilder depend on any of the My intuition is that this propose would not impact kubebuilder, but want to double check. |
e955197 to
f0ed9d8
Compare
|
|
||
| - **Main strategy**: clears status on create (`obj.Status = TypeStatus{}`), | ||
| and clears status changes on update (`new.Status = old.Status`). | ||
| - **Status substrategy**: clears spec, labels, and annotations changes |
There was a problem hiding this comment.
Would we phrase this to clear all new fields as they're added unless we take active effort? I'd like that so we're forced to think about it.
There was a problem hiding this comment.
Yes, good point. I'll rephrase this.
|
|
||
| This requires [Spec/Status accessor interfaces](#specstatus-accessor-interfaces). | ||
|
|
||
| ### Feature-Gate Field Dropping |
There was a problem hiding this comment.
This probably the most valuable part for me personally.
|
|
||
| **Hooks:** | ||
|
|
||
| | Hook | Purpose | |
There was a problem hiding this comment.
no Hook for PrepareForCreate or PrepareForUpdate?
There was a problem hiding this comment.
Yes, I'll call it out.
|
A few questions and comments. I like the idea. |
It depends on what you mean by "depends on", it does interpret them but that probably doesn't matter for in-tree resources as I don't think there is a good reason to use it to generate DeepCopies for in-tree resources: https://github.com/kubernetes-sigs/controller-tools/blob/e17f046d11b6e9e508d11abc37891b66831aefa4/pkg/deepcopy/gen.go#L45-L47 |
👍 |
Uh oh!
There was an error while loading. Please reload this page.